Increase modelinefmt configuration power #1267

danr · 2017-03-07T22:02:27Z

Two tests need to be updated because an empty string is now on display
when %val{context_info} from modelinefmt's
{Information}%val{context_info}{default} is empty.

casimir · 2017-03-08T08:00:41Z

Does it make the status line configurable? Can you give an example of possible configuration?

danr · 2017-03-08T08:34:49Z

After a good night's sleep I now conclude that this patch is a little funky, namely that it requires { in options and values to be escaped to not be interpreted as markup. Example using this patch, if your modelinefmt includes %val{bufname} and your buffer's name is hello{red}world, then it be rendered as helloworld, with world in blue.

The workaround I can muster right now is %sh[echo "${kak_bufname//{/\\{"] which works but requries a sh at each time the modeline is rewritten. What other alternatives are there? Could we support some kind of escaping in %val and %opt? An example using bash's horrible syntax: %val[bufname//{/\\{].

danr · 2017-03-08T08:40:56Z

@casimir It introduces two new values:

context_info: the part of the modeline that says things like [+][new_file][recording (@)]
input_handler_info: the part which says 1 sel param=42 and insert 4 sel

The latter is written with the markup that the modelinefmt option understand (same as echo -markup), like {StatusLineMode}insert {StatusLineInfo}1 sel{default}.

Using these values one can to customize the modeline. The standard one is then:

%val{bufname} %val{cursor_line}:%val{cursor_char_column} {Information}%val{context_info}{default} %val{input_handler_info} - %val{client}@[%val{session}]

(I now realize I should have documented this together with the patch.)

danr · 2017-03-08T13:12:52Z

Ok, and a screenshot:

casimir · 2017-03-08T13:20:32Z

That's nice! Still It needs some documentation.

lenormf · 2017-03-08T13:48:32Z

I think we would benefit from this more if all the atoms were splitted, not just spread into two sub groups.

danr · 2017-03-08T14:10:22Z

I am certainly willing to do that, but then I would like to make boolean values is_modified, is_new_file , no_hooks and int values param, and string values register and recording_register. It would become many vals (which I don't mind), but one would need shell to turn is_modified being true into [+]. Would this be a show-stopper? It would be a very long modelinefmt. Here's a sketch:

set global modelinefmt %{
    {Information}%sh{
        [[ "$kak_is_modified" -eq "true" ]] && echo [+]
        [[ "$kak_no_hooks" -eq "true" ]] && echo [no-hooks]
        # ... and a bunch more
    }
    {StatusLineMode}%val{mode}
    {StatusLineInfo}%sh{
    	if [[ "$kak_mode" -eq "insert" ]]; then
            echo $kak_num_sels sel
            [[ "$kak_count" -gt 0 ]] && echo {StatusLineInfo} param={StatusLineValue}$kak_count
            [[ "$kak_reg" -gt 0 ]] && echo {StatusLineInfo} reg={StatusLineValue}$kak_reg
        fi
        # ... and so on
    }
}

lenormf · 2017-03-08T16:11:18Z

Maybe there's no need to be so generic, you could make it so that each variable already contains the appropriate atom. Spawning one shell scope per atom wouldn't be very efficient, so I think there needs to be a trade off there; also the editor wasn't supposed to be 100% configurable in the first place, that's why this part of the status bar was simply rendered in one block, so a compromise has to be made to stay in line with the original idea.

danr · 2017-03-08T19:54:06Z

Thanks for your input @lenormf

Spawning one shell scope per atom wouldn't be very efficient,

Naturally only one is strictly needed (since %{%sh{X}Y%sh{Z}}=%{%sh{X; echo Y; Z}} is in this setting almost true for all X, Y, Z). I shouldn't have used several in my sketch above.

you could make it so that each variable already contains the appropriate atom

Here is a suggestion based on this then:

Splice	Example values
`%val{is_modified}`	`[+]` or empty
`%val{no_hooks}`	`[no-hooks]` or empty
...	some more similar ones
`%val{mode}`	`insert`/`menu`/`prompt`/`enter key`/empty for normal,
`%val{num_sels}`	the number of selections; `1`, `4`, ...
`%val{mode_params}`	`{StatusLineInfo} param={StatusLineValue}1234` and `{StatusLineInfo} reg={StatusLineValue}c` (or both after each other)

Also important: what about escaping values in for example %val{bufname} that could contain the markup-starter {?

danr · 2017-03-08T22:07:42Z

The introduced business with escaping { can be undone if we find a nice way to split mode_params into more vals without markup. In the table above they are the only ones that use markup.

One way would be to have a val which is param= if the param count is set, and the empty string otherwise. Say that this would be called param_prefix, then this part of the modeline would be {StatusLineInfo}%val{param_prefix}{StatusLineValue}%val{param}. And similar vals for the reg part.

mawww · 2017-03-08T22:33:46Z

I think the original separation is good enough, although I'd rename input_handler_info to mode_info.

The escaping problem is tricky, and I am not really keen on passing strings around that have to be parsed down the line when we know exactly what display line we want to generate.

I am not sure if there is a nice solution for that, but ideally mode_line should still return a DisplayLine, and we should not have to escape { in values. maybe by using specific logic to expand %...{...} directly to DisplayLines in parse_display_line instead of expanding then parsing.

danr · 2017-03-09T20:34:21Z

Updated this now. It even has documentation!

I went for a solution of the escaping problem with a special markup atom {``mode_info} which expands into a DisplayLine after all other escaping has happened. The %val{context_info} part which contains things like [fifo][recording (@)] gets its markup from the modelinefmt option.

Thanks all for your feedback, I hope this gets accepted.

mawww · 2017-03-09T23:49:43Z

src/client.cc

- [](String s) { return escape(s, '{', '\\'); }));
+ auto expanded = expand(modelinefmt, context(), ShellContext{},
+ [](String s) { return escape(s, '{', '\\'); });
+ auto m_atoms = HashMap<String, DisplayLine>{{ "mode_info", context().client().input_handler().mode_line() }};


m_ is a prefix we use to mark class fields, here m_atoms is a local variable, so it should just be named atoms. Also, as the right hand side is not a function call, I'd rather see that written HashMap<String, DisplayLine> atoms{...};.

mawww · 2017-03-09T23:53:12Z

src/main.cc

@@ -154,7 +156,24 @@ void register_env_vars()
 "window_height", false,
 [](StringView name, const Context& context) -> String
 { return to_string(context.window().dimensions().line); }
- } };
+ }, {
+ "context_info", false,


I dont think it makes much sense for that to be exposed as a value, I'd rather see it in the atoms hash map.

mawww · 2017-03-10T00:05:01Z

src/display_buffer.cc

@@ -312,7 +317,16 @@ DisplayLine parse_display_line(StringView line)
 auto closing = std::find(it+1, end, '}');
 if (closing == end)
 throw runtime_error("unclosed face definition");
- face = get_face({it+1, closing});
+ if (*(it+1) == '`')


Cant say I like this syntax, but to be faire I dont like the general display line syntax {face}...

I think this is a good occasion to revise it, I am not sure what would be better, but I'd like to get closer to something more well known... Unfortunately none of Markdown, asciidoc, restructuredText seems to provide a syntax for face/color selection, so the remaining reference I can think of is LaTeX but its not very satisfying either.

Any idea on what could be a nice syntax here, at least supporting changing face colors and referencing a built-in value, maybe more in the future as we'll probably want to get better formatting support for info boxes at some point.

In the javascript world, templates often use the mustache notation, so you're not too far off with the current markup.

danr · 2017-03-10T09:23:31Z

Thank you for the review, I changed the markup to {{mode_info}} instead, it doesn't look as ugly.

(I was under the impression that m_ was a prefix for maps!)

Delapouite · 2017-03-10T10:24:39Z

What does m_ variable prefix mean?

http://stackoverflow.com/questions/13018189/what-does-m-variable-prefix-mean

I stumbled upon the same question while reading C++ sources.

lenormf · 2017-03-10T10:47:10Z

I remember this being a part of the google style guide for C++, back when it was first published. Must have changed since.

mawww · 2017-03-10T11:20:45Z

src/display_buffer.cc

@@ -312,7 +317,18 @@ DisplayLine parse_display_line(StringView line)
 auto closing = std::find(it+1, end, '}');
 if (closing == end)
 throw runtime_error("unclosed face definition");
- face = get_face({it+1, closing});
+ if (*(it+1) == '{' and *(closing+1) == '}')


closing+1 might be the end.

mawww · 2017-03-10T11:41:31Z

src/display_buffer.cc

+ return parse_display_line_with_atoms(line, HashMap<String, DisplayLine>{});
+}
+
+DisplayLine parse_display_line_with_atoms(StringView line, HashMap<String, DisplayLine> atoms)


I dont think we need the two functions, just parse_display_line taking a hash map argument with a defaultu value of = {}.

mawww · 2017-03-10T11:42:06Z

src/client.cc

@@ -124,32 +125,30 @@ DisplayLine Client::generate_mode_line() const
 {
 const String& modelinefmt = context().options()["modelinefmt"].get<String>();

- modeline = parse_display_line(expand(modelinefmt, context(), ShellContext{},
- [](String s) { return escape(s, '{', '\\'); }));
+ String context_info = "";


Move the context_info display line building into a helper function.

danr · 2017-03-11T19:25:31Z

src/client.cc

+ s += "[no-hooks]";
+ if (context.buffer().flags() & Buffer::Flags::Fifo)
+ s += "[fifo]";
+ return s;


I have just realized I don't know C++. Where is this string allocated? When will it be deleted? Is it safe to return it like this? It is sent to parse_display_line below.

The String class takes care of all that, if its small enough (<= 22 bytes), it should end up embeded in the string object, else it will end up heap allocated, but the String is taking care of the ownership of that allocation, so this code is safe.

In Kakoune code, and generally in modern style C++, these problems are solved, its only the building block classes that should take care of memory allocation/freeing and provide a safe abstraction that prevent leaks.

mawww

Yet another set of remarks, sorry for missing them in the first place.

mawww · 2017-03-11T19:32:58Z

src/display_buffer.cc

@@ -286,7 +286,7 @@ void DisplayBuffer::optimize()
 line.optimize();
 }

-DisplayLine parse_display_line(StringView line)
+DisplayLine parse_display_line(StringView line, HashMap<String, DisplayLine> atoms)


Use a const HashMap<...>& here, as we only access the map for reading, so there is no reason to copy the hash map.

mawww · 2017-03-11T19:35:01Z

src/display_buffer.cc

- face = get_face({it+1, closing});
+ if (*(it+1) == '{' and closing+1 != end and *(closing+1) == '}')
+ {
+ auto atom_it = atoms.find(String{it+2, closing});


Use a StringView here, no need to copy the data.

mawww · 2017-03-11T19:35:22Z

src/display_buffer.cc

+ {
+ auto atom_it = atoms.find(String{it+2, closing});
+ if (atom_it == atoms.end())
+ throw runtime_error(format("undefined atom {}", String{it+2, closing}));


same, StringView here

mawww · 2017-03-11T19:37:42Z

src/display_buffer.cc

+ auto atom_it = atoms.find(String{it+2, closing});
+ if (atom_it == atoms.end())
+ throw runtime_error(format("undefined atom {}", String{it+2, closing}));
+ for (auto display_it = atom_it->value.begin(), end = atom_it->value.end(); display_it != end; ++display_it)


This is a for each, you can just write it for (auto& atom : atom_it->value) res.push_back(atom);

mawww · 2017-03-11T19:41:00Z

src/display_buffer.hh

@@ -143,7 +144,7 @@ private:
 AtomList m_atoms;
 };

-DisplayLine parse_display_line(StringView line);
+DisplayLine parse_display_line(StringView line, HashMap<String, DisplayLine> m_atoms = HashMap<String, DisplayLine>{});


m_atoms shoud just be atoms, and I think atom is not such a good name, I'd suggest built_ins, or named_values. Also, the default value can just be written {}, no need to repeat the type.

danr · 2017-03-11T20:44:22Z

No problem :) Thanks for reviewing and info about C++ 👍 !

Delapouite mentioned this pull request Mar 9, 2017

Show keys in the status line while recording macros. #1273

Open

danr force-pushed the modelinefmt branch from 0be5d9a to d041a3a Compare March 9, 2017 20:31

mawww reviewed Mar 10, 2017

View reviewed changes

danr force-pushed the modelinefmt branch from d041a3a to 84d0b44 Compare March 10, 2017 09:22

mawww requested changes Mar 10, 2017

View reviewed changes

danr force-pushed the modelinefmt branch 2 times, most recently from f31a464 to 5e5dd52 Compare March 10, 2017 12:57

danr commented Mar 11, 2017

View reviewed changes

mawww requested changes Mar 11, 2017

View reviewed changes

Increase modelinefmt configuration power

5a403a9

danr force-pushed the modelinefmt branch from 5e5dd52 to 5a403a9 Compare March 11, 2017 20:43

mawww merged commit 5a403a9 into mawww:master Mar 12, 2017

danr deleted the modelinefmt branch May 15, 2017 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase modelinefmt configuration power #1267

Increase modelinefmt configuration power #1267

danr commented Mar 7, 2017

casimir commented Mar 8, 2017

danr commented Mar 8, 2017

danr commented Mar 8, 2017

danr commented Mar 8, 2017

casimir commented Mar 8, 2017

lenormf commented Mar 8, 2017

danr commented Mar 8, 2017

lenormf commented Mar 8, 2017

danr commented Mar 8, 2017

danr commented Mar 8, 2017

mawww commented Mar 8, 2017

danr commented Mar 9, 2017

mawww Mar 9, 2017

mawww Mar 9, 2017

mawww Mar 10, 2017

lenormf Mar 10, 2017

danr commented Mar 10, 2017

Delapouite commented Mar 10, 2017

lenormf commented Mar 10, 2017

mawww Mar 10, 2017

mawww Mar 10, 2017

mawww Mar 10, 2017

danr Mar 11, 2017

mawww Mar 11, 2017

mawww left a comment

mawww Mar 11, 2017

mawww Mar 11, 2017

mawww Mar 11, 2017

mawww Mar 11, 2017

mawww Mar 11, 2017

danr commented Mar 11, 2017

Increase modelinefmt configuration power #1267

Increase modelinefmt configuration power #1267

Conversation

danr commented Mar 7, 2017

casimir commented Mar 8, 2017

danr commented Mar 8, 2017

danr commented Mar 8, 2017

danr commented Mar 8, 2017

casimir commented Mar 8, 2017

lenormf commented Mar 8, 2017

danr commented Mar 8, 2017

lenormf commented Mar 8, 2017

danr commented Mar 8, 2017

danr commented Mar 8, 2017

mawww commented Mar 8, 2017

danr commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danr commented Mar 10, 2017

Delapouite commented Mar 10, 2017

lenormf commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mawww left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danr commented Mar 11, 2017